-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix 743 multi file and 744 debsign support #782
Conversation
545c40d
to
c847624
Compare
Pull Request Test Coverage Report for Build 7412
💛 - Coveralls |
aa0b072
to
ee12150
Compare
r? @ajvb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking awesome! Great tests and docs are making this a breeze to review!
This is just a first pass. Tomorrow I want to spend a bit more time reading through the core of the multiple signer and gpg code, but has some initial CR comments and questions I thought I'd share right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Just have two comments related to the use of os.RemoveAll()
in the new gpg signing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(re-approving after new commits dismissed last approval)
TY! I still need to squash this PR too. |
6ada7d2
to
d575751
Compare
to avoid leaving them on disk
d575751
to
25589ce
Compare
fix #743 multiple file signing
fix #744 debsign support
Note: this is a combined PR since an interface without an implementation wasn't terribly useful.
Changes:
Implement support for signing multiple files #743
files
field to the signature request format in 570472esigned_files
field to the signature response format in 2a964b8This adds the name field to the proposal in #743, because
debsign
needs the file extension. Restrictions on the name field and number of files is documented here and aim to limit the potential for path/directory traversal and DoS.Signers that shell out have to decide how to map the named files to arguments. It doesn't provide a way to group or partition args for commands that take multiple variable args e.g.
cmd --json-inputs a.json b.json c.json ... --csv-input d.csv e.csv ...
, but our two immediate use cases don't require that flexibility.where
NamedUnsignedFile
andNamedSignedFile
encode the signing operation in types to prevent misuse and are both aliased to the marshaled version of the named file format:/sign/files
as originally proposedAdd a debsign mode to the gpg2 signer #744
debsign
signing mode that:gpg.conf
file (because we can't pass args to gpg2 via debsign); is cleaned up in the at exit handlergpg
orgpg2
/sign/files
for end users (/sign/data
will on sign the hardcoded monitoring input).commands
extension.debsign
accepts .commandsfiles, but [we don't use them](https://github.com/mozilla-services/autograph/issues/744#issuecomment-933661284) and it appears to support running
rm` and other commands, which we'd like to avoid.gpg2
mode when one is not providedOther changes
--no-options
and--homedir
set to the keyring temp dir to key load and detach sign gpg commands and run them from keyring temp dir to avoid creating and using/app/.gnupg
for gpg hometools/autograph-client
about multiple input files and add the-outfilesprefix
option